[ast/opa parse] Support marshalling of all ast location data #5576

charlieegan3 · 2023-01-19T15:17:31Z

This is being done so that the json representation of an ast parse can
be used more easily by automated tools which need the location
information. This location information is verbose for human users of the
data and has until now been excluded from JSON representation.

This PR makes it possible to configure the JSON marshalling with some
options. However, the functionality of the OPA parse command has been
left mostly unchanged.

The only with the json output is that the fields are in a different
order:

diff --git a/new.json b/old.json
index 5b9504fa..0fb9d4f2 100644
--- a/new.json
+++ b/old.json
@@ -13,15 +13,6 @@
   },
   "rules": [
     {
-      "body": [
-        {
-          "index": 0,
-          "terms": {
-            "type": "boolean",
-            "value": true
-          }
-        }
-      ],
       "head": {
         "name": "allow",
         "value": {
@@ -34,17 +25,26 @@
             "value": "allow"
           }
         ]
-      }
+      },
+      "body": [
+        {
+          "terms": {
+            "type": "boolean",
+            "value": true
+          },
+          "index": 0
+        }
+      ]
     }
   ],
   "comments": [
     {
+      "Text": "IGNvbW1lbnQ=",
       "Location": {
         "file": "main.rego",
         "row": 3,
         "col": 1
-      },
-      "Text": "IGNvbW1lbnQ="
+      }
     }
   ]
 }

It's also possible to now show locations for all terms with another flag
option.

$ go run main.go parse main.rego --format=json --json-include=locations
{
  "package": {
    "location": {
      "file": "main.rego",
      "row": 1,
      "col": 1
    },
    "path": [
      {
        "location": {
          "file": "main.rego",
          "row": 1,
          "col": 9
        },
        "type": "var",
        "value": "data"
      },
...
}

Reviewer Notes:

Following [ast/opa parse] Support marshalling of all ast location data #5576 (comment), I've left the comment json formatting as is. This can be picked up another time.
Following [ast/opa parse] Support marshalling of all ast location data #5576 (comment), I've renamed the flag to json-include
Following, [ast/opa parse] Support marshalling of all ast location data #5576 (comment), I've adjusted the implementation to use an option passed at compile time to control the JSON marshalling behaviour. This means that modules parsed with different options could have different JSONs.

netlify · 2023-01-19T15:18:45Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`4ea1566`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/63c95f0fb667290008fc35d1
😎 Deploy Preview	https://deploy-preview-5576--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

ast/policy.go

ast/annotations.go

charlieegan3 · 2023-01-19T17:21:13Z

I have some ideas about how to solve the mutation here and am still working on it.

cmd/parse.go

netlify · 2023-01-23T12:42:02Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`2c38a7c`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/64099e4be7b3e9000820d293
😎 Deploy Preview	https://deploy-preview-5576--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

charlieegan3 · 2023-01-23T17:18:41Z

ast/annotations.go

+		data["schemas"] = a.Schemas
+	}
+
+	if len(a.Custom) > 0 {


There are many checks here to preserve the omit empty functionality.

charlieegan3 · 2023-01-23T17:20:05Z

ast/annotations_test.go

-	// data.foo at foo.rego:5 has annotations {"scope":"subpackages","organizations":["Acme Corp."]}
-	// data.foo.bar at mod:3 has annotations {"scope":"package","description":"A couple of useful rules"}
+	// data.foo at foo.rego:5 has annotations {"organizations":["Acme Corp."],"scope":"subpackages"}
+	// data.foo.bar at mod:3 has annotations {"description":"A couple of useful rules","scope":"package"}


With the new marshalling functions, some of the field orderings have changed.

charlieegan3 · 2023-01-23T17:22:22Z

ast/parser.go

@@ -176,6 +179,12 @@ func (p *Parser) WithSkipRules(skip bool) *Parser {
 	return p
 }

+// WithJSONFields sets the JSON fields that should be exposed in the JSON
+func (p *Parser) WithJSONFields(jsonFields map[string]bool) *Parser {


Not sure if JSONFields would be better as a custom options type. e.g. ASTJSONMarshalOptions which then each AST struct could read from to determine how to marshal itself...

Hmm.. as opposed to this being global and applied to all nodes?

I could have been clearer here. I was considering changing the type of the jsonFields from map[string]bool to a custom struct for this use case, such as ASTJSONMarshalOptions.

ASTJSONMarshalOptions could wrap settings for global options and individual types. E.g. allow the changing of the json marshalling behaviour for a particular type only.

Hmmm... what would be an example use-case where we'd want to change the marshalling? 🤔

Sometimes we might want to have more control over which fields on which types are enabled / disabled.

For example, we might want to turn on locations for every only. This would be marginally easier to retrofit with a custom struct rather than a map[string]bool.

map[string]bool{ "every_location": true, }

ASTJSONMarshalOptions{ EveryLocations: true, }

Mostly because it'd reduce the number of presence checks in the map and make it easier to make targeted adjustments to certain types.

Replacing the map in JSONFields map[string]bool with a custom struct would give us the flexibility to expand the feature. We can always start with the global options and then later add in more fine-grained controls.

Another thought is do we need a map of node field type -> expose or not. We can simply not expose by default (ie. current behavior) and then only do so when explicitly set.

Ok thanks. I think that makes sense to me too. I'll have a go at this now.

do we need a map of node field type -> expose or not.

Perhaps it makes sense to do this now as part of an options wrapper which might grow in future. I'll see what it looks like.

I've had a go at this in ~~e15a39b~~ 64b5764, let me know what you think 🙂

charlieegan3 · 2023-01-23T17:27:24Z

ast/term.go

+	return nil
+}
+
+func unmarshalLocation(loc *Location, v map[string]interface{}) error {


I have added this since I needed to expand the functionality of term unmarshalling now that a location might have been included in the output data. I'm not sure if this is correct, perhaps it should not be possible to unmarshal and get the location data back in.

If the only thing that this could affect is where error locations are reported for example, then I don't think it'll be an issue to leave in. If anyone else sees an issue though, I'll defer to them on this.

charlieegan3 · 2023-01-23T17:30:41Z

Thanks all for the first round of reviews, I've reworked this following a suggestion from Ash which I think is a big improvement. Let me know what you think :)

ashutosh-narkar

@charlieegan3 this looks good. Few questions to clarify my understanding of the changes.

ashutosh-narkar · 2023-01-23T21:52:26Z

ast/marshal_test.go

+		Term         *Term
+		ExpectedJSON string
+	}{
+		"base case": {


So the base case and location excluded cases are the same ie by default we won't include location.

Yes, that's correct 🙂 This should be the case for all types.

ashutosh-narkar · 2023-01-23T22:03:52Z

ast/marshal_test.go

+				return &Import{
+					Path:     &term,
+					Location: NewLocation([]byte{}, "example.rego", 1, 2),
+					jsonFields: map[string]bool{


Nit: The other tests do not include the jsonFields field in the output which is fine I guess.

Yeah, that's a good spot. It's not needed and is inconsistent. I have removed it in 8e2fe4e

ashutosh-narkar · 2023-01-23T22:19:04Z

ast/parser.go

@@ -176,6 +179,12 @@ func (p *Parser) WithSkipRules(skip bool) *Parser {
 	return p
 }

+// WithJSONFields sets the JSON fields that should be exposed in the JSON
+func (p *Parser) WithJSONFields(jsonFields map[string]bool) *Parser {


Hmm.. as opposed to this being global and applied to all nodes?

ashutosh-narkar · 2023-01-23T22:32:15Z

ast/policy.go

+	}
+
+	// TODO: Comment Location is also always included for legacy reasons. jsonFields data is ignored.
+	if c.Location != nil {


Nit: Is this check needed if the location is also always included ?

Hmm, that's a good point. In order to leave this functionality as it is I have changed things around in ee24358.

I think it's clear enough to note this inconsistency on the struct and leave out the no-op custom marshal function for now. I have left the tests in however.

ashutosh-narkar · 2023-01-23T22:37:51Z

cmd/parse.go

@@ -77,5 +105,7 @@ func parse(args []string, stdout io.Writer, stderr io.Writer) int {

 func init() {
 	parseCommand.Flags().VarP(parseParams.format, "format", "f", "set output format")
+	parseCommand.Flags().StringVarP(&parseParams.jsonInclude, "json-include", "", "", "select optional elements, current options: locations, comments. E.g. --json-include locations,-comments will include locations and exclude comments.")


I'm curious why comments was included as a --json-include option. Is it because it's easy to remove from the parsed module or something else?

That's a good question. I added it for two reasons:

Comments use an inconsistent JSON key name where the fields are capitalised for legacy reasons. For this reason, a use of the command might want to drop them if they are interested in walking the JSON and processing it in an automated way.

I wanted to include an additional option beyond locations for this flag so that it was clear how it was to be used.

Clearly the latter reason isn't very strong and I'd be more than happy to remove this functionality. I just thought it make it clearer in the PR how that flag was used.

philipaconrad

Note: @charlieegan3, I'd suggest waiting to merge until @ashutosh-narkar has had a chance to review this, but I'm marking approve because this looks solid to me.

LGTM! 🙂 I don't see anything wild or terribly unexpected here, although I have 1-2 small curiosity questions. In particular, I'd like to see whether or not we can get the Comment/comment spelling inconsistency resolved eventually! 😅

philipaconrad · 2023-01-30T21:03:13Z

ast/marshal_test.go

+	}
+}
+
+// TODO: Comment has inconsistent JSON field names starting with an upper case letter. Comment Location is


[question]: Possibly a silly question, but is there a way for us to signal that the "legacy" capitalization is being deprecated, so that we can eventually change the field naming to the correct version?

Feel free to ignore if this is out-of-scope for this PR!

I'm not sure how to do that in a good way, perhaps we could log to stderr but I'm not sure tbh.

philipaconrad · 2023-01-30T21:05:02Z

ast/parser.go

@@ -176,6 +179,12 @@ func (p *Parser) WithSkipRules(skip bool) *Parser {
 	return p
 }

+// WithJSONFields sets the JSON fields that should be exposed in the JSON
+func (p *Parser) WithJSONFields(jsonFields map[string]bool) *Parser {


Hmmm... what would be an example use-case where we'd want to change the marshalling? 🤔

philipaconrad · 2023-01-30T21:12:50Z

ast/term.go

+	if generatedRaw, ok := v["generated"]; ok {
+		if b, ok := generatedRaw.(bool); ok {
+			expr.Generated = b
+		} else {
+			return fmt.Errorf("ast: unable to unmarshal generated field with type: %T (expected true or false)", v["generated"])
+		}
+	}
+


[question/curiosity]: What motivated this case? 😃 I'm always happy to see good defensive programming land in core utility functions, but this case is not entirely obvious from its surroundings. Is it caused by generated rules from ast/compiler or somesuch?

My understanding on what generated: true means is elementary. However, I think it's that during compilation, some terms are rewritten and replaced with generated terms, perhaps to 'de-sugar' or some other reason.

Why this is here is that in order to round trip marshal and unmarshal an expression, this unmarshal logic was missing. It's not really related to the main functionality of this PR.

philipaconrad · 2023-01-30T21:19:30Z

ast/term.go

+	return nil
+}
+
+func unmarshalLocation(loc *Location, v map[string]interface{}) error {


If the only thing that this could affect is where error locations are reported for example, then I don't think it'll be an issue to leave in. If anyone else sees an issue though, I'll defer to them on this.

ashutosh-narkar

@charlieegan3 this is getting close 😄 Just had few comments around how we define the fields to expose.

ashutosh-narkar · 2023-02-15T23:07:37Z

ast/marshal_test.go

+	// text is not marshalled to JSON so we just drop it in our examples
+	rule.Location.Text = nil
+
+	testCases := map[string]struct {


Not sure I understand the difference between the base and location case.

Also in general can you please help me with what are you testing with the Test*_UnmarshalJSON tests?

Not sure I understand the difference between the base and location case.

The idea here was to show that by default, the locations were not included AND that they were not included when the flag was set to false explicitly. It is perhaps unnecessary!

Also in general can you please help me with what are you testing with the Test*_UnmarshalJSON tests?

Before this PR, a number of types didn't support the unmarshalling of location data found in their JSON representations. As the PR evolved, this become only relevant for the Term and Expr types. However the tests remain for all. We could strip out the unmarshal tests for all types without custom unmarshalling. See the helper unmarshalLocation in the diff for where this is used.

I have removed the unmarshal tests for all node types other than Term and Expr in ~~2ce4b93~~ 4997590

ashutosh-narkar · 2023-02-16T00:36:54Z

ast/parser.go

@@ -176,6 +179,12 @@ func (p *Parser) WithSkipRules(skip bool) *Parser {
 	return p
 }

+// WithJSONFields sets the JSON fields that should be exposed in the JSON
+func (p *Parser) WithJSONFields(jsonFields map[string]bool) *Parser {


Replacing the map in JSONFields map[string]bool with a custom struct would give us the flexibility to expand the feature. We can always start with the global options and then later add in more fine-grained controls.

Another thought is do we need a map of node field type -> expose or not. We can simply not expose by default (ie. current behavior) and then only do so when explicitly set.

charlieegan3 · 2023-03-01T14:50:40Z

I've rebased this again today. I'm pretty happy with the implementation now, would you be able to have another quick look @ashutosh-narkar?

cc @anderseknert

ashutosh-narkar

LGTM @charlieegan3! Can you please add a test in cmd/parse_test.go ?

ashutosh-narkar · 2023-03-01T22:39:17Z

ast/term.go

@@ -444,6 +455,14 @@ func (term *Term) UnmarshalJSON(bs []byte) error {
 		return err
 	}
 	term.Value = val
+


Nit: We should update the func desc to something like Specialized unmarshalling is required to handle Value and location

Good spot, I've added this in fea4dbf

charlieegan3 · 2023-03-02T11:51:57Z

Thanks for the review Ash, I've made some changes to the parse command tests in 2c571c2. I had to make some edits to the parse function to allow the options to be passed in. Hope that all looks good now 🤞

anderseknert

This is awesome! And much awaited 💯 🚀 👏

One thing I'm curious about, and I don't see any references to it in the PR: the rego.parse_module built-in function. It's an outlier in many ways, and I am most likely its only user 😄 From my understanding of the code here, the output of that built-in function will not change with this. Correct? It'd be useful if that would emit location data too, but I'm not sure if we can have it changed. And we can't really change the function signature to allow for configuration options either. Might have to build a custom function for my needs, which is fine :)

OTOH, I'm not sure that adding data to its AST output would be considered breaking..

anderseknert · 2023-03-02T12:01:43Z

cmd/parse_test.go

+        "value": {
+          "location": {
+            "file": "TEMPDIR/x.rego",
+            "row": 3,


It's interesting that we get the location of the value, but not of the var. I guess it's easy to deduce that it'll be on the row 0? :)

Indeed 😞 The reason for this is that Var is 'just' a string and has no other internal state so we'd have no where to store the location data or JSON options to render it. I'm not sure there is a lot we can do here without making some pretty major changes to the way the ast package works.

@tsandall any ideas?

cmd/parse.go

charlieegan3 · 2023-03-02T15:42:33Z

output of that built-in function will not change with this. Correct?

Yeah that function doesn't use any options at the moment. The default in this PR is to leave the json marshalling as it is but alter the options available. In the opa parse we set ProcessAnnotation but leave all other parser options off. In the built in no options are set which is arguably inconsistent.

we can't really change the function signature to allow for configuration options

Given this, I think it might be best to use a custom function for our use case 😉

Part of: #3143 This is being done so that the json representation of an ast parse can be used more easily by automated tools which need the location information. This location information is verbose for human users of the data and has until now been excluded from JSON representation. This PR makes it possible to configure the JSON marshalling with some options. However, the functionality of the OPA parse command has been left mostly unchanged. The only with the json output is that the fields are in a different order: ``` diff --git a/new.json b/old.json index 5b9504fa..0fb9d4f2 100644 --- a/new.json +++ b/old.json @@ -13,15 +13,6 @@ }, "rules": [ { - "body": [ - { - "index": 0, - "terms": { - "type": "boolean", - "value": true - } - } - ], "head": { "name": "allow", "value": { @@ -34,17 +25,26 @@ "value": "allow" } ] - } + }, + "body": [ + { + "terms": { + "type": "boolean", + "value": true + }, + "index": 0 + } + ] } ], "comments": [ { + "Text": "IGNvbW1lbnQ=", "Location": { "file": "main.rego", "row": 3, "col": 1 - }, - "Text": "IGNvbW1lbnQ=" + } } ] } ``` It's also possible to now show locations for all terms with another flag option. ``` $ go run main.go parse main.rego --format=json --json-include=locations { "package": { "location": { "file": "main.rego", "row": 1, "col": 1 }, "path": [ { "location": { "file": "main.rego", "row": 1, "col": 9 }, "type": "var", "value": "data" }, { "location": { "file": "main.rego", "row": 1, "col": 9 }, "type": "string", "value": "foo" } ] }, "rules": [ { "body": [ { "index": 0, "terms": { "type": "boolean", "value": true } } ], "head": { "name": "allow", "value": { "location": { "file": "main.rego", "row": 5, "col": 9 }, "type": "boolean", "value": true }, "ref": [ { "type": "var", "value": "allow" } ] } } ], "comments": [ { "Location": { "file": "main.rego", "row": 3, "col": 1 }, "Text": "IGNvbW1lbnQ=" } ] } ``` Signed-off-by: Charlie Egan <charlie@styra.com>

Signed-off-by: Charlie Egan <charlie@styra.com>

This change adds a new struct JSONOptions which allows the JSON marshalling and unmarshalling of ast nodes to be further customized in the future. It's my hope that this will be more extendable than the simple map we were using before. Signed-off-by: Charlie Egan <charlie@styra.com>

Before this PR, a number of types didn't support the unmarshalling of location data found in their JSON representations. As the PR evolved, this become only relevant for the Term and Expr types. Other unmarshal tests are removed in this commit. Signed-off-by: Charlie Egan <charlie@styra.com>

This adds tests for: * pretty output * json output * json location and comment flags This required the parse function to be adjusted to make the options configurable in test cases. Signed-off-by: Charlie Egan <charlie@styra.com>

Signed-off-by: Charlie Egan <charlie@styra.com>

charlieegan3 commented Jan 19, 2023

View reviewed changes

ast/policy.go Outdated Show resolved Hide resolved

charlieegan3 mentioned this pull request Jan 19, 2023

opa parse: Allow source location to be provided in output #3143

Closed

charlieegan3 commented Jan 19, 2023

View reviewed changes

ast/annotations.go Outdated Show resolved Hide resolved

charlieegan3 marked this pull request as draft January 19, 2023 17:20

ashutosh-narkar reviewed Jan 19, 2023

View reviewed changes

cmd/parse.go Show resolved Hide resolved

charlieegan3 commented Jan 23, 2023

View reviewed changes

charlieegan3 marked this pull request as ready for review January 23, 2023 17:29

ashutosh-narkar reviewed Jan 23, 2023

View reviewed changes

philipaconrad previously approved these changes Jan 30, 2023

View reviewed changes

anderseknert mentioned this pull request Feb 3, 2023

Release v0.49.0 #5626

Closed

charlieegan3 dismissed philipaconrad’s stale review via dc4f68a February 6, 2023 09:35

ashutosh-narkar reviewed Feb 16, 2023

View reviewed changes

charlieegan3 mentioned this pull request Feb 22, 2023

Two more rules StyraInc/regal#15

Merged

ashutosh-narkar previously approved these changes Mar 1, 2023

View reviewed changes

charlieegan3 dismissed ashutosh-narkar’s stale review via 0cb0edc March 2, 2023 11:45

anderseknert previously approved these changes Mar 2, 2023

View reviewed changes

charlieegan3 dismissed anderseknert’s stale review via 876c1eb March 2, 2023 15:35

ashutosh-narkar previously approved these changes Mar 7, 2023

View reviewed changes

ashutosh-narkar dismissed their stale review via 743e327 March 8, 2023 08:12

ashutosh-narkar previously approved these changes Mar 8, 2023

View reviewed changes

anderseknert dismissed ashutosh-narkar’s stale review via b70c0c1 March 9, 2023 08:38

charlieegan3 added 8 commits March 9, 2023 09:52

Explain comment location marshalling behaviour

66a0e20

Signed-off-by: Charlie Egan <charlie@styra.com>

Remove surplus jsonFields data

59b5ee7

Signed-off-by: Charlie Egan <charlie@styra.com>

Improve parse command tests

c7bea96

This adds tests for: * pretty output * json output * json location and comment flags This required the parse function to be adjusted to make the options configurable in test cases. Signed-off-by: Charlie Egan <charlie@styra.com>

Clarify term UnmarshalJSON functionality

b872438

Signed-off-by: Charlie Egan <charlie@styra.com>

Make default flag behavior clearer

2c38a7c

Signed-off-by: Charlie Egan <charlie@styra.com>

anderseknert approved these changes Mar 9, 2023

View reviewed changes

charlieegan3 merged commit fbfa705 into open-policy-agent:main Mar 9, 2023

charlieegan3 deleted the parse-locations branch March 9, 2023 09:49

charlieegan3 mentioned this pull request Apr 12, 2023

[ast/inspect] Annotation location data missing from opa inspect #5826

Closed

anderseknert mentioned this pull request Sep 6, 2023

AST: Allow jsonOptions to include Text field of Location struct #6213

Closed

[ast/opa parse] Support marshalling of all ast location data #5576

[ast/opa parse] Support marshalling of all ast location data #5576

Conversation

charlieegan3 commented Jan 19, 2023 • edited Loading

netlify bot commented Jan 19, 2023

✅ Deploy Preview for openpolicyagent ready!

charlieegan3 commented Jan 19, 2023

netlify bot commented Jan 23, 2023 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 commented Jan 23, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipaconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 commented Mar 1, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

charlieegan3 commented Mar 2, 2023 • edited Loading

anderseknert left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlieegan3 commented Mar 2, 2023

charlieegan3 commented Jan 19, 2023 •

edited

Loading

netlify bot commented Jan 23, 2023 •

edited

Loading

charlieegan3 Feb 23, 2023 •

edited

Loading

charlieegan3 Jan 24, 2023 •

edited

Loading

charlieegan3 Feb 23, 2023 •

edited

Loading

charlieegan3 Mar 2, 2023 •

edited

Loading

charlieegan3 commented Mar 2, 2023 •

edited

Loading

anderseknert left a comment •

edited

Loading